Repository navigation
CKS: reconcile node capacity after live resize - #14223
andrijapanicsb wants to merge 14 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14223 +/- ##
============================================
+ Coverage 20.45% 20.78% +0.33%
- Complexity 21415 21979 +564
============================================
Files 6487 6499 +12
Lines 589303 591417 +2114
Branches 72245 72545 +300
============================================
+ Hits 120525 122913 +2388
+ Misses 455643 455160 -483
- Partials 13135 13344 +209
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
andrijapanicsb
changed the title
|
@ACSHomeBot package G |
Packaging results:
Test packages are available at: |
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.88% |
| Branch coverage | 20.27% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
40b50b0 to
7e5a95a
Compare
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Queued for worker G. Jobs ahead: 0. |
|
Package building started on worker G. |
Packaging results:
Test packages are available at: |
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Queued for worker G. Jobs ahead: 0. |
|
Package building started on worker G. |
Packaging results:
Test packages are available at: |
andrijapanicsb
changed the title
|
The technical findings are fair, and I’ve addressed them. The remark about instructing “my AI”, however, is unnecessary. Please keep the review focused on concrete issues in the code. I’m contributing here as an operator because the existing live-resize path could successfully resize a VM while Kubernetes continued to advertise the old resources. Whatever the original implementation’s scope, operators still had to complete the Kubernetes-side work manually. A successful VM resize is not, by itself, a successful Kubernetes resize. There is some irony in focusing on a non-developer’s use of AI when that contribution is closing this end-to-end gap. Clean coding principles are a fair requirement; complete behaviour and end-to-end testing deserve the same scrutiny. Please judge the implementation and its verification—not the contributor’s background or authoring tools. |
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Completed. Packaging results. |
Packaging results:
Test packages are available at: |
|
[SF] Trillian test result (tid-17260)
|
Pearl1594
left a comment
Pearl1594
left a comment
There was a problem hiding this comment.
This is a good improvement. Thanks @andrijapanicsb . I've provided some comments. Also, there might be a little clash with #14138
ok, then please keep to clean code principles for future submissions. (This is not personally to you Andrija and stating things like this is very necessary in this project. Also not personally to you. I do not know what AI you use to generate your code, hence "your" AI!) |
- Move the per-node resize into KubernetesClusterNodeOfferingResizer and make the reconciler a plain class with small top-level value classes. - Mark a node as pending and cordon it in one conditional kubectl patch, and finish or release it with one patch too. Conflicts, rejected patches and lost replies are settled by reading the node again, a limited number of times. - Keep the pending marker after any failure. Remove the CKS cordon only when the VM did not reach the target offering, so a retry of the same scale finishes the work. A VM already on the target offering is skipped after one control-plane read unless it still has unfinished work. - Refuse the resize before changing anything when the guest already has more CPUs or memory than the target offering. - Check guest memory by its online memory blocks instead of MemTotal. MemTotal leaves out what the kernel keeps for itself, which grows with the memory the VM booted with, so the old margin rejected healthy nodes of about 5 to 20 GiB, for example on a CPU-only change. - Restart kubelet only when the node is not up to date or kubelet is not active, and check that allocatable is between zero and capacity instead of comparing with an earlier snapshot. - Catch RuntimeException at the per-node boundary again, as on main, so an unexpected error still cleans up and marks the scale as failed. - Resize dedicated etcd VMs the same way as on main. - Reach a node through the IP of its own SSH port-forwarding rule, which fixes Netris VPC tiers. - Share the per-type offering lookup, include removed offerings for the current offering and the cluster totals, and check the offerings of all affected node types before any VM is resized. - Check the scale timeout before each node instead of after it. - Revert an unrelated whitespace change in the Spring context.
|
Hi @Pearl1594, thanks a lot for the detailed review, I really appreciate the time you put into it. I've pushed the changes in 306cc2e and replied to each of your comments above. While working through them I also found two problems I had missed: a node cordoned by an operator would never have been retried after a failure, and the node SSH address was wrong on Netris VPC tiers. Both are fixed now. In two places I went a slightly different way from what you suggested (the etcd check and the skip on SSH failure), and I explained why in those threads. Let me know if anything still looks off. |
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Completed. Packaging results. |
- Name the repeated READ_GUEST, metadata and status strings once. - Use \d in the guest and Kubernetes quantity patterns. - Move the node type selection of the offering preflight into its own method, so the loop has no continue statements. - Drop throws Exception from two tests that cannot throw it.
|
@blueorangutan package |
|
@andrijapanicsb a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
@DaanHoogland, unrelated to all this work - share with @rajujith some of the PR fixes that I've raised against virt-v2v that are critical for VMware migrations, I'm sure Jithin will appreciate those - https://github.com/libguestfs/virt-v2v/pulls?q=is%3Apr+author%3Aandrijapanicsb - all merged, and he can just take the source DEB/RPM (whatever your worker uses) and backport those, together with the stupid FSTRIM that virt-v2v does - I had a 30TB migration over weekened, and FSTRIM added 2h of duration inside virt-v2v on top of 200 seconds that it really took to convert a WIndows VM with 12 disks/30TB of space. Perhaps he wants to also backport the --no-fstrim option from upstream. @rajujith ping me if you want to test openvddk.com - it's NBD/NBDSSL and HotAdd - that is a 1:1 binary compatible replacement for VMware's VDDK - completely free. I'm sure you might want to test it in your in-house product. |
@andrijapanicsb , looks like a compatitor of the one mentioned in https://www.networkworld.com/article/4241265/fluidcloud-replaces-vddk-with-its-own-disk-copy-engine-for-vmware-exits.html needs a comparison? (in a discussion item, probably not here) |
Packaging results:
Test packages are available at: |
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Completed. Packaging results. |
|
|
code looks very clean now @andrija, thnks. |
@DaanHoogland you know me well enough - don't compare apples and oranges... ;) "The FluidCloud engine exports the VM disk using Network File Copy (NFC) through standard application programming interfaces (API) on the VMware ESXi hypervisor. It then streams the virtual machine disk (VMDK) file over the network to the destination. Omar said the export captures everything, with no functionality loss. The engine does not do delta sync, so every export is a full export of the VM." Their stuff is a workaorund but uses NFS, My artefact is the libvixdisk.so.8.0.3 that you replace and off you go (HotAdd, NBD/NBDDSSL (via NFC) - 100% compatible with VDDK = so all tools previously designed to work with VMware VDDK now works with OpenVDDK. check the sites, numbers, compatibilityy etc. I will share this publicly, soon. Free and opensource. |
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Completed. Packaging results. |
Packaging results:
Test packages are available at: |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19802 |
Packaging results:
Test packages are available at: |
Context
#13226 added live compute-offering changes for CKS nodes, building on #12975. The VM could gain CPU/RAM while Kubernetes still reported the old capacity. Operators had to activate guest resources and restart
kubeletmanually. #14213 tracks that gap.Changes
CKS now activates and verifies the added guest CPU/RAM, restarts
kubeletwhen needed, and waits for the Node to report the new capacity and allocatable resources before completing the scale operation.Nodes are handled one at a time without draining resident Pods. While a node's resize is in progress, the node carries a
cloudstack.apache.org/cks-live-resize-pendingannotation and, unless an operator had already cordoned it, a CKS cordon. Both are set and removed with single conditional Node updates. If a scale fails, the annotation stays, and the CKS cordon stays too if the VM already reached the new offering. A retry of the same scale finishes that node before it changes any other. An existing operator cordon is preserved.QA also exposed a pre-existing cluster CPU/RAM accounting bug, not a regression from this PR. Totals are now calculated from the configured per-node-type offerings and counts, so the API and UI reflect mixed control/worker offerings correctly.
Functional and regression testing
Test environment: Cloud-managed KVM, isolated network, published packages from
f92e52d736b9, before the review changes. The tests will be repeated on the final commit.VPC/direct-access and multi-control HA were not tested. Dedicated etcd VMs are resized the same way as on main; they are not Kubernetes nodes, so nothing is reconciled on them (not live-tested). The separate existing
nodeids-only scale-in issue found during QA is tracked in #14458, with its fix in #14459.UI evidence
Successful scale task and correct aggregate capacity:
Scale request and result after Refresh
Control RAM: 2048 → 3072 MiB; worker unchanged at 3 CPU / 4096 MiB.
After Refresh: 6 CPU / 7168 MiB.